Seed cipher lists and version bounds from the system crypto policy - #3503
Conversation
952a912 to
6c7c325
Compare
| // Seeding is best-effort, so the errors its failures queue must not reach the | ||
| // caller. Neither may the caller's own queue be disturbed, since this runs | ||
| // inside |SSL_CTX_new|, which no caller expects to touch the error queue at | ||
| // all. Cutting the queue back to the length it had leaves everything below the | ||
| // cut alone: the caller's entries, the data pointer their last | ||
| // |ERR_get_error_line_data| handed out, and any mark they set. It allocates | ||
| // nothing, so it has no failure mode, and if seeding queued nothing it does | ||
| // nothing. | ||
| const size_t num_errors = ERR_num_errors(); |
There was a problem hiding this comment.
Reason not to use the ERR_save_state?
There was a problem hiding this comment.
That was the approach i took initially, preserving mark state across copies. Claude found a few issues with that:
ERR_save_statenull return value indicates both queue empty + OOM- saving the mark across copies (effectively re-setting the mark on every restore) had some unintended consequences elsewhere in the code base
- performance -- 2 additional malloc's on every
SSL_CTX_newif error queue non-empty
There was a problem hiding this comment.
Moved to error queue suppression in #3529
| // Marked with OPENSSL_EXPORT to make it available for unit tests. | ||
| OPENSSL_EXPORT bool ssl_crypto_policy_parse_file(const char *path, | ||
| CryptoPolicyConfig *out); | ||
|
|
||
| // ssl_crypto_policy_default_path returns the path of the crypto-policies OpenSSL | ||
| // back-end file to read: the value of the AWSLC_CRYPTO_POLICY_FILE environment | ||
| // variable if set and non-empty, otherwise the compile-time | ||
| // |AWSLC_CRYPTO_POLICY_DEFAULT_FILE|. Mirrors the SSL_CERT_FILE override idiom. | ||
| // | ||
| // The environment override is ignored in processes running with elevated | ||
| // privileges, where the environment sits on the far side of a privilege boundary | ||
| // from the root-owned default path. | ||
| const char *ssl_crypto_policy_default_path(void); | ||
| // | ||
| // Marked with OPENSSL_EXPORT to make it available for unit tests. | ||
| OPENSSL_EXPORT const char *ssl_crypto_policy_default_path(void); | ||
|
|
||
| // ssl_ctx_apply_crypto_policy seeds |ctx| from the crypto-policies OpenSSL | ||
| // back-end file at |path|. It is best-effort and never fails: a missing or | ||
| // malformed file, or a directive AWS-LC rejects, leaves the corresponding | ||
| // built-in default in place. Errors already queued by the caller are preserved; | ||
| // errors this function provokes are not. | ||
| // | ||
| // |is_dtls| selects the TLS.* vs DTLS.* protocol directives. |version_locked| | ||
| // must be true when |ctx| came from one of the legacy version-locked | ||
| // |SSL_METHOD|s (|ssl_method_st.version| non-zero), in which case the policy's | ||
| // protocol floor and ceiling are skipped: the caller pinned a single version and | ||
| // a system-wide default must not silently widen it. | ||
| // | ||
| // The parsed file is cached process-wide, keyed on |path|, so repeated | ||
| // |SSL_CTX_new| calls do not re-read it. | ||
| // | ||
| // Marked with OPENSSL_EXPORT to make it available for unit tests. | ||
| OPENSSL_EXPORT void ssl_ctx_apply_crypto_policy(SSL_CTX *ctx, const char *path, | ||
| bool is_dtls, | ||
| bool version_locked); |
There was a problem hiding this comment.
With -DENABLE_DIST_PKG=ON -DBUILD_SHARED_LIBS=ON -DENABLE_CRYPTO_POLICIES=ON, ssl_test fails to link on Linux with undefined references to all three helpers.
OPENSSL_EXPORTis not sufficient andssl/libssl.map'slocal: *;hides anything missing from the registry. (Disabling symbol versioning makes the same build link.)- You should register these as
PRIVATE_CXXinssl/libssl.txtand regenerate the map (or keep them private and test through public APIs?). The extractor also needs anAWSLC_CRYPTO_POLICIESconfiguration; its default scan misses these declarations.
There was a problem hiding this comment.
Ah, this configuration set was missing from our CI job. Fixed symbol export and added additional CI coverage (shared libs, dist. pkg, symbol versioning + crypto policy support) in #3527
|
Also address the secure-execution and partial-read issues noted on #3502. |
Stack, split out of #3442, to merge bottom-up: 1. **#3501 -- error-queue primitives (this PR)** 2. #3502 -- policy file reader 3. #3503 -- cipher lists and version bounds 4. #3504 -- groups and signature algorithms 5. #3505 -- post-quantum defaults ## Description - Adds `ERR_num_errors` and `ERR_pop_to_count`, so code that calls into libcrypto on a caller's behalf can drop the errors it raised and leave the queue it was handed untouched. - The existing mark APIs cannot do this. `ERR_set_mark` needs an entry to mark, so it is a no-op on an empty queue, and popping to a mark consumes one the caller had already set. - `ERR_clear_error` and `ERR_restore_state` rebuild the queue, which dangles the data pointer the caller got from its last `ERR_get_error_line_data`. - A count is a position rather than a mark, so it nests inside a caller's mark without disturbing it. ## Testing / verification - New error-queue tests cover popping back to a recorded count, a count taken from an empty queue, and a count at or above the queue's length. - One case fills the ring past its capacity to confirm a stale count leaves the caller's errors alone. - One case wraps a nested call in the caller's own mark and an error carrying a data string, then checks the mark still pops and the string is intact. - One case pins that a mark does not survive `ERR_save_state`/`ERR_restore_state`, since a snapshot can be restored many times and would re-arm a mark nobody set. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.
6c7c325 to
e12618d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3503 +/- ##
==========================================
- Coverage 78.41% 78.21% -0.20%
==========================================
Files 700 700
Lines 125889 125885 -4
Branches 17413 17410 -3
==========================================
- Hits 98711 98464 -247
- Misses 26305 26549 +244
+ Partials 873 872 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
390a732 to
cb08448
Compare
|
🔒 Security Review — View Report Please review before merging. |
cb08448 to
4c8d99d
Compare
4c8d99d to
1e92d17
Compare
06b7fcf to
be7981b
Compare
be7981b to
8a4942a
Compare
Stack, split out of #3442, to merge bottom-up (#3501, the error-queue primitives, has landed): 1. **#3502 -- policy file reader (this PR)** 2. #3527 -- shared-build symbol export and CI 3. #3503 -- cipher lists and version bounds 4. #3504 -- groups and signature algorithms 5. #3505 -- post-quantum defaults ## Description - Adds an off-by-default build flag, `-DENABLE_CRYPTO_POLICIES`, and a reader for the OpenSSL back-end file that Amazon Linux 2023 and Fedora render from the operator's chosen system policy. - `AWSLC_CRYPTO_POLICY_FILE` relocates that file at build time and is declared in the CMake cache, so `cmake -L` and cmake-gui list it for a packager who is not reading the CMakeLists. - Parses the directives AWS-LC could act on into a fixed-size config struct. Nothing consumes the result yet. - Comments, section headers, and unknown keys are ignored, so the reader tolerates the rest of what the framework writes today and whatever it adds later. A value too long to represent is ignored the same way, even where an earlier line set the same key. - The reader succeeds only if it read the whole file, so half a policy cannot pass for a shorter one. - The policy path is fixed at build time and overridable at runtime, which is how the tests here and in the rest of the stack drive it. The override is dropped in a secure execution, where the environment sits on the far side of a privilege boundary from the root-owned default path. ## Testing / verification - Parse tests cover a full stock policy, quoting, surrounding whitespace, a repeated key, and a final line with no trailing newline. - An overlong value or line is dropped whole rather than truncated, and drops the value an earlier line gave the same key, so a policy larger than the reader's buffers cannot quietly become a different policy. - A missing file, a read error, and null arguments all fail rather than yielding a half-filled config. The read-error case opens a directory, which fails on the first read rather than on the open. - The runtime path override is exercised directly, since every later test in the stack rests on it. - Checked the override drop against a real binary: honored as an ordinary process, ignored once that binary carries file capabilities, which leave the real and effective ids equal. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.
Stack, split out of #3442, to merge bottom-up (#3501, the error-queue primitives, has landed): 1. #3502 -- policy file reader 2. **#3527 -- shared-build symbol export and CI (this PR)** 3. #3503 -- cipher lists and version bounds 4. #3504 -- groups and signature algorithms 5. #3505 -- post-quantum defaults ## Description - Exports the two policy-reader internals the tests call and registers them in the libssl symbol registry. A shared build needs both: hidden visibility keeps them out of the library, and the version script an `ENABLE_DIST_PKG` build applies keeps them out again. - Runs the symbol extractor once more with the crypto-policies build flag defined, so declarations sitting behind that guard reach the registry at all. - Adds the Amazon Linux 2023 CI job for the feature: the suite with the flag on in stock CMake and in the shared, symbol-versioned `ENABLE_DIST_PKG` build a distribution packages, a run against the policy file the system renders, and a build with libssl off. - The job's seeding-specific parts, the neutralizing path override and the require-system flag, are inert until #3503 adds seeding and the test hook that reads them. - The build flag stays off by default, so no shipped configuration changes. ## Testing / verification - Ran both build configurations the job runs and the policy tests in each. Only the distribution one fails to link without this change, which is why a stock build alone let the gap through. - Reverting only the generated version script brings the undefined references back, so the registry entries earn their place alongside the export attribute. - Regenerating the version script from the registry is byte-identical, the invariant the symbol-check job's `mapcheck` mode enforces. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.
SSL_CTX_new now applies the policy's CipherString, Ciphersuites, and protocol bounds when built with ENABLE_CRYPTO_POLICIES. Seeding is best-effort, so it cannot report failure and must leave the caller's error queue as it found it. Two directives cannot be applied blind. A failing SSL_CTX_set_cipher_list installs its empty result before returning, so a rule AWS-LC cannot satisfy would leave the context with no ciphers rather than the defaults. The version setters validate each bound only against the method's whole range, so an inverted pair would be accepted and fail every later handshake.
The policy file was re-read on every SSL_CTX_new; cache the parse keyed on the resolved path so a changed AWSLC_CRYPTO_POLICY_FILE still misses. The three internal entry points need OPENSSL_EXPORT to reach ssl_test across a shared libssl, which -fvisibility=hidden otherwise blocks. A temp-file write failure now fails the test instead of skipping it; the platform skip moves to the fixtures.
Every existing case composes its own fixture, so nothing noticed that the framework's Groups value carries a '*' key-share marker and its Ciphersuites value names a suite AWS-LC lacks. The fixture now copies Amazon Linux 2023's DEFAULT file verbatim, and a new suite runs against whatever the host installed: the AL2023 CI job sets AWSLC_CRYPTO_POLICY_TEST_REQUIRE_SYSTEM, so an absent file fails there and skips elsewhere.
A caller that reaches SSL_CTX_new with a full error queue loses its oldest entry to the first error seeding raises, and popping back to the count taken beforehand cannot return it. Rejecting the errors at the source keeps the queue bit-for-bit as seeding found it however full it was.
ERR_num_errors is going away with ERR_pop_to_count, and draining the queue proves what the count stood in for.
Neither cipher setter is a no-op on failure: the list parser installs an empty result before reporting that a rule matched nothing, and the merge that follows allocates, so a context could keep part of a policy it could not apply. The version setters are independent of each other and each can refuse. Seeding is skipped outright when the suppression scope cannot be opened, since running it unprotected would leak errors to the caller.
8a4942a to
08a8c3a
Compare
The built-in floor is TLS 1.0, below every floor crypto-policies can render, so dropping a floor AWS-LC cannot resolve left the context offering versions the policy forbids.
A cached failure let one transient read error decide the policy for every SSL_CTX the process went on to create.
…3504) Stack, split out of #3442, to merge bottom-up. Below the stack, #3501 and #3529 (error-queue primitives and the suppression scope), #3502 (policy file reader), and #3527 (shared-build symbol export and CI) have landed. 1. #3503 -- cipher lists and version bounds 2. **#3504 -- groups and signature algorithms (this PR)** 3. #3505 -- comparison documentation ## Description - Extends seeding to the policy's groups and signature algorithms, narrowed to the algorithms AWS-LC implements and kept in the operator's order. Every stock policy names something AWS-LC does not have -- X448, the FFDHE groups, Ed448, RSA-PSS-PSS, the SHA-224 pairs -- and the setters reject a whole list on the first unknown name, so applying a value as written would discard the preference order entirely. - Reads the OpenSSL 3.5 list syntax the stock policies use: `secp256r1` for NIST P-256, `*` and `?` on an entry, `/` as an entry boundary, and `-` to remove. Left as written, the two most-preferred groups in every Amazon Linux 2023 list are the ones that throw the list away. - A `-` removes its algorithm whatever else the value names, and takes the ML-KEM hybrid over a removed group with it, because the hybrid performs the key exchange the operator just forbade. A value that only removes is applied to AWS-LC's own default list, in either directive, and one that leaves nothing at all is skipped, since an empty list is how AWS-LC asks for its defaults. - Keeps AWS-LC's post-quantum defaults when the policy says nothing about them, as Amazon Linux 2023's `DEFAULT` does not; the setters replace AWS-LC's defaults rather than intersect with them, so seeding would otherwise downgrade every context. A policy naming any post-quantum algorithm is taken at its word, and one that names an algorithm only to remove it keeps that algorithm out of what is restored. `AWSLC.PostQuantum = off`, a directive of AWS-LC's own, waives the defaults, since nothing the framework writes says "no post-quantum"; it goes in a `crypto-policies` drop-in file, because the framework rewrites the generated back-end file on every policy change. - Adds an internal name-to-signature-algorithm lookup that probes the existing list parser under `bssl::ScopedErrorSuppression` and reports the name unresolvable when the queue cannot be protected, so a rejected name is never observable in the caller's queue. AWS-LC signs with a different default list than it accepts, so a `SignatureAlgorithms` value is resolved against each of them. The two preference lists are separate allocations, so the signing list is moved aside and put back, leaving the defaults in force when the verify setter fails. ## Testing / verification - Amazon Linux 2023's `DEFAULT` and `DEFAULT:PQ` renderings are exercised verbatim and the seeded lists asserted exactly, in policy order, including where the hybrids and ML-DSA fall. They are the only stock values that stack modifiers, separate tuples with `/`, and spell each post-quantum algorithm twice. - Removal is covered in both directives: alongside entries the same value keeps, on its own against the default list, removing every group so the directive is skipped, on an ML-DSA algorithm the post-quantum defaults would otherwise restore, and on Ed25519, which only the signing default list carries. - The post-quantum rules are covered in both directions -- a silent policy keeps the defaults, a policy naming one is left as written -- with the opt-out exercised case-insensitively and against a removal-only `Groups` value. - The test that reads the host's own policy file resolves it through the seeding path rather than a parser written in the test, so a spelling the resolver loses cannot pass unnoticed. - Each guard was checked by neutering it in turn: the P-256 translation, the modifier handling, the de-duplication, the narrowing, the opt-out, and the classical-half requirement each fail a named test. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.
Stack, split out of #3442, to merge bottom-up. Below this PR, #3501 and #3529 (error-queue primitives and the suppression scope), #3502 (policy file reader), and #3527 (shared-build symbol export and CI) have landed.
Description
SSL_CTX_newnow applies the policy's cipher lists and protocol bounds when built with the flag, taking the TLS or DTLS directives according to the context's method.bssl::ScopedErrorSuppression, which rejects the errors it raises at the source and is false when the queue cannot be protected, where seeding is skipped. Trimming afterwards is not enough for a caller whose queue is already full: there each error seeding raises evicts one of theirs, and no later trim brings an evicted entry back.crypto-policiesrenders, so a context would offer the versions the policy exists to forbid. A floor naming a protocol older than any AWS-LC implements keeps the built-in one, which is already stricter, and a ceiling AWS-LC cannot resolve stays unapplied.@SECLEVEL=Nprefix ofCipherStringis parsed and dropped, since AWS-LC has no security levels; the key-size and hash constraints a level implies are therefore not enforced.openssl.cnf: a policy change takes effect only in processes started afterward, while a changed override path still misses.open()perSSL_CTX_newon a host with no policy.Testing / verification
crypto-policiesitself renders, comparing the automatically seeded context against one seeded by hand from the same path; the require-system flag this PR teaches the tests to honor turns an absent file into a failure rather than a skip.CipherStringthe policy applies is checked to leave the TLS 1.3 suites in place, and aCiphersuitesvalue the TLS 1.2 ciphers, which is the merge seeding now performs itself.ERR_get_error_line_data, a queue filled to capacity, and one with a single slot free.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.